Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a generic spin loader #389

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

aaravlu
Copy link
Contributor

@aaravlu aaravlu commented Feb 12, 2025

issue: #387
Spin loader's size could follow its View's size:
Screencast from 2025-02-12 13-09-29.webm

@aaravlu aaravlu marked this pull request as ready for review February 12, 2025 05:11
@aaravlu aaravlu added the waiting-on-review This issue is waiting to be reviewed label Feb 12, 2025
Copy link
Contributor

@ZhangHanDong ZhangHanDong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RobrixSpinLoader component is essentially well-designed to provide a dynamic spinning loading effect. With some optimizations and flexibility enhancements, such as adding dynamic transparency, configurable segment count, and rotation speed control, it can be made more customizable and efficient. Additionally, considering performance, the calculation process can be further streamlined to avoid unnecessary complex calculations during animation frames.

@ZhangHanDong ZhangHanDong added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Feb 13, 2025
@aaravlu aaravlu requested a review from kevinaboos February 14, 2025 02:07
@aaravlu aaravlu added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Feb 14, 2025
@aaravlu
Copy link
Contributor Author

aaravlu commented Feb 14, 2025

15 segments with 0.1 speed:
Screencast from 2025-02-14 10-09-56.webm

5 segments with 1.0 speed:
Screencast from 2025-02-14 10-10-40.webm

@ZhangHanDong
Copy link
Contributor

lgtm

Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Aarav, this is a nice addition. I have a few comments:

  1. Since this is not Robrix-specific, I think it belongs as an addition to Makepad itself, not Robrix. Then we can use it within Robrix.
  2. I showed the animation to a few colleagues, and we all tend to agree that it's very aggressive/jarring, especially at higher speeds. It's almost uncomfortable to watch, as a user waiting on something. Instead, we should use a more relaxed, standard loading spinner animation. I would recommend the "indeterminate circular progress indicator" here, number 2 at this link: https://m3.material.io/components/progress-indicators/guidelines#af0eec4b-39d6-4311-b101-aef0ae6c7078, which is used by both Android and iOS.
  3. The size of the spinner should not scale up with the size of the view. If you have a huge full-screen view, such as your ImageViewer, you don't want to show a giant loading spinner that is 800 pixels wide. We still want to show a relatively small one, say 80 pixels in size. Thus, the spinner should scale down to fit smaller views, but should not be larger than a certain maximum size. (This is similar to my comment about Image sizes here)

@kevinaboos kevinaboos removed the waiting-on-review This issue is waiting to be reviewed label Feb 21, 2025
@aaravlu
Copy link
Contributor Author

aaravlu commented Feb 21, 2025

Thanks Aarav, this is a nice addition. I have a few comments:

Thanks for advice.

I fully agree with your 1 & 2 points.

But i think we should make it follow it's father View's size, it could automatically select a maxmium square to show the circle ('cause the circle is a standard circle).

That is, our purpose is to give other developers in robrix space to customize the circle size, you can add View, set flow to Overlay and set width & height to customize any size you want.
Our purpose is to make the widget more generic.

@aaravlu
Copy link
Contributor Author

aaravlu commented Feb 21, 2025

Since this is not Robrix-specific, I think it belongs as an addition to Makepad itself, not Robrix. Then we can use it within Robrix.

Got, i would transfer this pr to makepad.

I would improve it here, if ok, i shall pr it to makepad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants